-
Notifications
You must be signed in to change notification settings - Fork 92
feat(serialization): Add GraalVM metadata configuration #1905
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(serialization): Add GraalVM metadata configuration #1905
Conversation
d5356e3
to
4c9956d
Compare
Hey @jreijn, is this PR ready for review? |
Hi @phipag, yes I believe this PR is ready for review. |
Awesome, thanks again for your work here @jrein. As you can see, there are a couple of pending PRs at the moment that I need to review but I'll do my best to test this soon. |
Thanks @phipag. Don't worry, I understand. I'm still working on powertools-batch, so I will just keep filling your backlog 😉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Congrats @jreijn on another GraalVM PR. This looks great. I just left two small comments. Can you commit those and resolve the merge conflict?
After this we are good to merge. I tested both examples end-to-end using GraalVM in my account and it worked like a charm.
Thanks for your patience waiting for my review 🚀
examples/powertools-examples-serialization/sam-graalvm/README.md
Outdated
Show resolved
Hide resolved
examples/powertools-examples-serialization/sam-graalvm/README.md
Outdated
Show resolved
Hide resolved
50ec7ad
to
b88ad1f
Compare
Fix documentation url Co-authored-by: Philipp Page <[email protected]>
Made URL for curl command generic instead of containing specific ids or regions Co-authored-by: Philipp Page <[email protected]>
b88ad1f
to
af1ecf0
Compare
…atch other dependencies
Thanks @phipag, I've made the requested changes. However I noticed that with the update of the runtime-client-interface dependency the example is broken due to some classes that got removed and the native libraries have been changed. Currently fixing testing that again to see what I need to do to get it in a working state. It might be that some other graalvm examples are broken as well at the moment. If I get it fixed I will create a new PR for those other modules when I get the metadata right. |
Thanks and good catch @jreijn. I'll wait here for your update. Please ping me once it is ready for review again. Also looking forward to the fixes for the other modules. I will also work on this issue soon which will allow us to catch such regressions for the production modules in the future. |
…me-client changes
|
@phipag ready for review. ✅ I'll create an issue for the other modules that already have GraalVM metadata. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🥳 . Works in my AWS account.
Issue #, if available: #1839
Description of changes:
This PR changed:
Breaking change checklist
RFC issue #:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.